-
-
Notifications
You must be signed in to change notification settings - Fork 496
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(traverse)!: remove unsound APIs #7514
Conversation
Your org has enabled the Graphite merge queue for merging into mainAdd the label “0-merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix. You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link. |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
CodSpeed Performance ReportMerging #7514 will not alter performanceComparing Summary
|
d21c0db
to
28a8277
Compare
I wrote almost all this last week. Just finishing it up and getting it into git now before my computer dies! |
@Boshen It's a bit annoying having to have There are alternatives which would also be safe, but they'd be less performant and require more boilerplate code. |
Merge activity
|
It's essential to `oxc_traverse`'s safety scheme that the user cannot create a `TraverseAncestry`, because they could then substitute it for the one stored in `TraverseCtx`, and cause a buffer underrun when an ancestor gets popped off stack which should never be empty - but it is because user has sneakily swapped it for another one. Not being able to create a `TraverseAncestry` also requires that user cannot obtain an owned `TraverseCtx` either, because you can obtain an owned `TraverseAncestry` from an owned `TraverseCtx`. Therefore, it's unsound for `TraverseCtx::new` to be public. However, it is useful in minifier to be able to re-use the same `TraverseCtx` over and over, which requires having an owned `TraverseCtx`. To support this use case, introduce `ReusableTraverseCtx`. It is an opaque wrapper around `TraverseCtx`, which prevents accessing the `TraverseCtx` inside it. It's safe for user to own a `ReusableTraverseCtx`, because there's nothing they can do with it except for using it to traverse via `traverse_mut_with_ctx`, which ensures the safety invariants are upheld. At some point, we'll hopefully be able to reduce the number of passes in the minifier, and so remove the need for `ReusableTraverseCtx`.But in the meantime, this keeps `Traverse`'s API safe from unsound abuse. Note: Strictly speaking, there is still room to abuse the API and produce UB by initiating a 2nd traversal of a different AST in an `Traverse` visitor, and then `mem::swap` the 2 x `&mut TraverseCtx`s. But this is a completely bizarre thing to do, and would basically require you to write malicious code specifically designed to cause UB, so it's not a real risk in practice. We'd need branded lifetimes to close that hole too. So this PR doesn't 100% ensure safety in a formal sense, but it at least makes it very hard to trigger UB *by accident*, which was the risk before.
28a8277
to
84e1bf6
Compare
It's essential to `oxc_traverse`'s safety scheme that the user cannot create a `TraverseAncestry`, because they could then substitute it for the one stored in `TraverseCtx`, and cause a buffer underrun when an ancestor gets popped off stack which should never be empty - but it is because user has sneakily swapped it for another one. Not being able to create a `TraverseAncestry` also requires that user cannot obtain an owned `TraverseCtx` either, because you can obtain an owned `TraverseAncestry` from an owned `TraverseCtx`. Therefore, it's unsound for `TraverseCtx::new` to be public. However, it is useful in minifier to be able to re-use the same `TraverseCtx` over and over, which requires having an owned `TraverseCtx`. To support this use case, introduce `ReusableTraverseCtx`. It is an opaque wrapper around `TraverseCtx`, which prevents accessing the `TraverseCtx` inside it. It's safe for user to own a `ReusableTraverseCtx`, because there's nothing they can do with it except for using it to traverse via `traverse_mut_with_ctx`, which ensures the safety invariants are upheld. At some point, we'll hopefully be able to reduce the number of passes in the minifier, and so remove the need for `ReusableTraverseCtx`.But in the meantime, this keeps `Traverse`'s API safe from unsound abuse. Note: Strictly speaking, there is still room to abuse the API and produce UB by initiating a 2nd traversal of a different AST in an `Traverse` visitor, and then `mem::swap` the 2 x `&mut TraverseCtx`s. But this is a completely bizarre thing to do, and would basically require you to write malicious code specifically designed to cause UB, so it's not a real risk in practice. We'd need branded lifetimes to close that hole too. So this PR doesn't 100% ensure safety in a formal sense, but it at least makes it very hard to trigger UB *by accident*, which was the risk before.
84e1bf6
to
5143bb8
Compare
It's essential to `oxc_traverse`'s safety scheme that the user cannot create a `TraverseAncestry`, because they could then substitute it for the one stored in `TraverseCtx`, and cause a buffer underrun when an ancestor gets popped off stack which should never be empty - but it is because user has sneakily swapped it for another one. Not being able to create a `TraverseAncestry` also requires that user cannot obtain an owned `TraverseCtx` either, because you can obtain an owned `TraverseAncestry` from an owned `TraverseCtx`. Therefore, it's unsound for `TraverseCtx::new` to be public. However, it is useful in minifier to be able to re-use the same `TraverseCtx` over and over, which requires having an owned `TraverseCtx`. To support this use case, introduce `ReusableTraverseCtx`. It is an opaque wrapper around `TraverseCtx`, which prevents accessing the `TraverseCtx` inside it. It's safe for user to own a `ReusableTraverseCtx`, because there's nothing they can do with it except for using it to traverse via `traverse_mut_with_ctx`, which ensures the safety invariants are upheld. At some point, we'll hopefully be able to reduce the number of passes in the minifier, and so remove the need for `ReusableTraverseCtx`.But in the meantime, this keeps `Traverse`'s API safe from unsound abuse. Note: Strictly speaking, there is still room to abuse the API and produce UB by initiating a 2nd traversal of a different AST in an `Traverse` visitor, and then `mem::swap` the 2 x `&mut TraverseCtx`s. But this is a completely bizarre thing to do, and would basically require you to write malicious code specifically designed to cause UB, so it's not a real risk in practice. We'd need branded lifetimes to close that hole too. So this PR doesn't 100% ensure safety in a formal sense, but it at least makes it very hard to trigger UB *by accident*, which was the risk before.
It's essential to
oxc_traverse
's safety scheme that the user cannot create aTraverseAncestry
, because they could then substitute it for the one stored inTraverseCtx
, and cause a buffer underrun when an ancestor gets popped off stack which should never be empty - but it is because user has sneakily swapped it for another one.Not being able to create a
TraverseAncestry
also requires that user cannot obtain an ownedTraverseCtx
either, because you can obtain an ownedTraverseAncestry
from an ownedTraverseCtx
.Therefore, it's unsound for
TraverseCtx::new
to be public.However, it is useful in minifier to be able to re-use the same
TraverseCtx
over and over, which requires having an ownedTraverseCtx
.To support this use case, introduce
ReusableTraverseCtx
. It is an opaque wrapper aroundTraverseCtx
, which prevents accessing theTraverseCtx
inside it. It's safe for user to own aReusableTraverseCtx
, because there's nothing they can do with it except for using it to traverse viatraverse_mut_with_ctx
, which ensures the safety invariants are upheld.At some point, we'll hopefully be able to reduce the number of passes in the minifier, and so remove the need for
ReusableTraverseCtx
.But in the meantime, this keepsTraverse
's API safe from unsound abuse.Note: Strictly speaking, there is still room to abuse the API and produce UB by initiating a 2nd traversal of a different AST in an
Traverse
visitor, and thenmem::swap
the 2 x&mut TraverseCtx
s. But this is a completely bizarre thing to do, and would basically require you to write malicious code specifically designed to cause UB, so it's not a real risk in practice. We'd need branded lifetimes to close that hole too.So this PR doesn't 100% ensure safety in a formal sense, but it at least makes it very hard to trigger UB by accident, which was the risk before.